fix: support editable install with multiple roots #126
Conversation
|
Hi. 😄 My first attempt in PR here. |
src/griffe/finder.py
Outdated
| if line.startswith("MAPPING"): | ||
| paths = ast.literal_eval(line.split(" = ", 1)[1]) | ||
| assert isinstance(paths, dict) | ||
| return [Path(pt).parent for pt in paths.values()] |
There was a problem hiding this comment.
At this point I feel like we should parse the entire file with AST (without any evaluation), and iterate on the module's body to find the MAPPING assignment node. Who knows if setuptools won't split the assignment on multiple lines in some cases, or in the future?
It would look like this:
parsed_module = ast.parse(path.read_text())
for node in parsed_module.body:
if isinstance(node, ast.Assign) and node.targets[0].id == "MAPPING":
return [Path(constant.value).parent for constant in node.value.values]There was a problem hiding this comment.
I have changed to full AST parsing, I agree it will be more stable.
A concern I do have at this point is that the change by setuptools or editables or any other package will be bigger than that.
It is probably not for this MR, and I don't understand the ModuleFinder mechanism enough, but I think it might probably be augmented by using the sys.meta_hook, and in that case there is no need to specially handle each editable install variant.
If you prefer, I think the code below may replace the _extend_from_editable_modules
def find_package(self, module_name: str) -> Package | NamespacePackage: # noqa: WPS231
"""Find a package or namespace package.
Parameters:
module_name: The module name.
Raises:
ModuleNotFoundError: When the module cannot be found.
Returns:
A package or namespace package wrapper.
"""
filepaths = [
Path(module_name),
# TODO: handle .py[cod] and .so files?
Path(f"{module_name}.py"),
]
# Add meta path
specs_paths = []
for finder in sys.meta_path:
spec = finder.find_spec(module_name,None)
if spec and spec.submodule_search_locations:
specs_paths.extend(Path(path).parent for path in spec.submodule_search_locations)
namespace_dirs = []
for path in self.search_paths + specs_paths: # noqa: WPS440
path_contents = self._contents(path)
if path_contents:
for choice in filepaths:
abs_path = path / choice
if abs_path in path_contents:
if abs_path.suffix:
stubs = abs_path.with_suffix(".pyi")
return Package(module_name, abs_path, stubs if stubs.exists() else None)
else:
init_module = abs_path / "__init__.py"
if init_module.exists() and not _is_pkg_style_namespace(init_module):
stubs = init_module.with_suffix(".pyi")
return Package(module_name, init_module, stubs if stubs.exists() else None)
namespace_dirs.append(abs_path)
if namespace_dirs:
return NamespacePackage(module_name, namespace_dirs)
raise ModuleNotFoundError(module_name)There was a problem hiding this comment.
Thanks! For reference, where did you get that code from?
I am indeed thinking about replacing Griffe's whole package finding algorithm with a subclass of what the standard lib offers, but just like you, I don't understand it enough and was not able to conclude if it would fit Griffe's needs or not. But I'm definitely open to suggestions as that would drastically simplify the code base.
There was a problem hiding this comment.
This is a small change on the griffe code - just added the lines below (which I wrote myself) to the function from griffe, and iterated also on specs_paths
specs_paths = []
for finder in sys.meta_path:
spec = finder.find_spec(module_name,None)
if spec and spec.submodule_search_locations:
specs_paths.extend(Path(path).parent for path in spec.submodule_search_locations)
gilfree
force-pushed
the
editable-multiple
branch
from
a1b0390
to
034c0e6
Compare
Python packages may have multiple root modules where each is installed to its own folder under `site-packages`. When using new setuptools editable install in such cases griffe, when used by mkdocstrings fails to find the correct paths. The root cause it that griffe assumed a variable named `MAPPING` in the `.py` file created by the editable install, and assumed that this variable is a dict with single entry. When a package has multiple roots - then this dictionary contain multiple entries. This PR aims to handle such cases. I have no knowledge about `editables` so I could not fix that case. The unit test added simulates the case, and this has been tested against a real complex package with multiple roots using setuptools 65 editable install.
gilfree
force-pushed
the
editable-multiple
branch
from
034c0e6
to
61e9725
Compare
src/griffe/finder.py
Outdated
| if line.startswith("MAPPING"): | ||
| paths = ast.literal_eval(line.split(" = ", 1)[1]) | ||
| assert isinstance(paths, dict) | ||
| return [Path(pt).parent for pt in paths.values()] |
There was a problem hiding this comment.
I have changed to full AST parsing, I agree it will be more stable.
A concern I do have at this point is that the change by setuptools or editables or any other package will be bigger than that.
It is probably not for this MR, and I don't understand the ModuleFinder mechanism enough, but I think it might probably be augmented by using the sys.meta_hook, and in that case there is no need to specially handle each editable install variant.
If you prefer, I think the code below may replace the _extend_from_editable_modules
def find_package(self, module_name: str) -> Package | NamespacePackage: # noqa: WPS231
"""Find a package or namespace package.
Parameters:
module_name: The module name.
Raises:
ModuleNotFoundError: When the module cannot be found.
Returns:
A package or namespace package wrapper.
"""
filepaths = [
Path(module_name),
# TODO: handle .py[cod] and .so files?
Path(f"{module_name}.py"),
]
# Add meta path
specs_paths = []
for finder in sys.meta_path:
spec = finder.find_spec(module_name,None)
if spec and spec.submodule_search_locations:
specs_paths.extend(Path(path).parent for path in spec.submodule_search_locations)
namespace_dirs = []
for path in self.search_paths + specs_paths: # noqa: WPS440
path_contents = self._contents(path)
if path_contents:
for choice in filepaths:
abs_path = path / choice
if abs_path in path_contents:
if abs_path.suffix:
stubs = abs_path.with_suffix(".pyi")
return Package(module_name, abs_path, stubs if stubs.exists() else None)
else:
init_module = abs_path / "__init__.py"
if init_module.exists() and not _is_pkg_style_namespace(init_module):
stubs = init_module.with_suffix(".pyi")
return Package(module_name, init_module, stubs if stubs.exists() else None)
namespace_dirs.append(abs_path)
if namespace_dirs:
return NamespacePackage(module_name, namespace_dirs)
raise ModuleNotFoundError(module_name)
Python packages may have multiple root modules where each is installed to its own folder under
site-packages.When using new setuptools editable install in such cases griffe, when used by mkdocstrings fails to find the correct paths.
The root cause it that griffe assumed a variable named
MAPPINGin the.pyfile created by the editable install, and assumed that this variable is a dict with single entry.When a package has multiple roots - then this dictionary contain multiple entries.
This PR aims to handle such cases.
I have no knowledge about
editablesso I could not fix that case.The unit test added simulates the case, and this has been tested against a real complex package with multiple roots using setuptools 65 editable install.